-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Prepared States dependency should be Reference Counted #2769
Conversation
Visit the preview URL for this PR (updated for commit 75b9851): https://yew-rs-api--pr2769-prepared-rc-z5rk6w8p.web.app (expires Tue, 26 Jul 2022 12:59:14 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Size Comparison
✅ None of the examples has changed their size significantly. |
I'd argue that we should add a Clone bound on deps so they don't end up being double #[derive(Properties, PartialEq)]
struct CloneProps {
state: Vec<u8>,
}
#[function_component)]
fn Cloned(props: &CloneProps) -> Html {
let state = use_preapred_state!(async move |state: Rc<Vec<u8>>| { }}, props.state.clone()) // notice how this clone is required
html! {}
}
#[derive(Properties, PartialEq)]
struct RcProps {
state: Rc<Vec<u8>>,
}
#[function_component]
fn RcComp(props: &RcProps) -> Html {
let state = use_preapred_state!(async move |state: Rc<Rc<Vec<u8>>>| { /* now there's double Rc*/ }}, Rc::clone(&props.state)) // No clone of data is done
html! {}
}
#[function_component]
fn Parent() -> Html {
let state = use_state(Vec::<u8>::new)
html! {
<div>
<RcComp state={Rc::clone(&*state)} /> // this won't compile, but that is a another issue that we should deal with
<Cloned state={(*state).clone()} />
</div>
}
} Notice how we can avoid the clone by modifying This problem isn't limited to this hook either. It spans to other hooks as well, such as |
See: https://serde.rs/feature-flags.html#-features-rc I think we have 2 ways to solve this:
|
There's another option of making the parameter |
We cannot do this with To achieve this, we need a way to extract
Which needs specialisation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which needs specialisation.
That sucks. This looks good to me in the current state. Seems like exposing Rc<D>
is the best we can do
Description
When using prepared states,
Future
requires to be'static
so they can be spawned viaspawn_local
.Currently, prepared states accept a
FnOnce<&D> -> impl Future<T> + 'static
type as its closure type.If the closure is not rewritten, the lifetime of
&D
can be extended with cloning.However, as we rewrite prepared states from async closure to closure + async block automatically, users do not have the ability to add any statements between the closure and async block.
This pull request adjusts the signature so that it passes
Rc<D>
instead of&D
so the async block can be guaranteed to live'static
.Checklist